Skip to content

Change classof #1108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
Closed

Change classof #1108

wants to merge 10 commits into from

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Feb 18, 2016

No description provided.

Tests failures were caused by previous incomplete implementation
of classTag.
Now diagnoses missing ClassTags of abstract types
as implicit failures.

Also: Simpler API of tpd.clsOf.
If a classtag for `T` is available, a classtag
for `Array[T]` can also be generated.
Context bounds did not make it before into secondary
constructors. Now the evidence parameters generated
by context bounds get copied into secondary constructors.

Without this fix, scala.collection.immutable.PagedSeq fails
to compile in new classtag scheme.
Advantage: classOf is not magic anymore. It's also more
flexible as it gives us an easy way to go from a ClassTag
to a class.

For the moment we leave the Classof phase in, in order to
do the right thing if somebody references Predef.classOf
explicitly (our improved classOf is in DottyPredef). Once
we merge DottyPredef and Predef we can get rid of the phase.
Previously, the implicit definitions were always ambiguous, which
is surely not right.
Tests that classOf as is discriminating than the previous definition
in Scala (which was not implemented in dotty so far).
@DarkDimius
Copy link
Contributor

tests/run/classof.scala: classOf[Array[Unit]]
failed in runtime:

[info] java.lang.IllegalArgumentException
[info]  at java.lang.reflect.Array.newArray(Native Method)
[info]  at java.lang.reflect.Array.newInstance(Array.java:75)
[info]  at scala.reflect.ClassManifestDeprecatedApis$class.arrayClass(ClassManifestDeprecatedApis.scala:91)
[info]  at scala.reflect.AnyValManifest.arrayClass(Manifest.scala:68)
[info]  at scala.reflect.ClassTag$class.wrap(ClassTag.scala:47)
[info]  at scala.reflect.AnyValManifest.wrap(Manifest.scala:68)
[info]  at dotty.DottyPredef$.arrayTag(DottyPredef.scala:12)
[info]  at Test$.main(classof.scala:34)
[info]  at Test.main(classof.scala)
[info]  at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[info]  at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)

Cannot simply use `wrap` as it does the wrong thing for
Array[Unit] (what a mess!).
@sjrd
Copy link
Member

sjrd commented Feb 18, 2016

Turning classOf into a normal def with an implicit ClassTag is a tempting change, but it is has one major non-trivial consequence that needs that be thought through, at least: a tree for classOf[C] will not be a constant expression anymore!

This is particularly bad when it comes to annotations, as it won't be possible to use a classOf[C] in an annotation meant to be used by Java. This is very damaging for JUnit tests, for example, whose expect-exception-to-be-thrown tests are based on a classOf[SomeException] to be present in annotations.

@retronym
Copy link
Member

I also would be reluctant to make classOf differ from Java's class literals. I've argued against such a change in Scalac.

@DarkDimius
Copy link
Contributor

I agree that

classOf[T] is much nicer than implicitly[ClassTag[T]].runtimeClass.

I wouldn't be surprised if there will be some performance regressions if we follow the proposed scheme, as it would become harder for JVM to speculate\discover what particular java.lang.Class is being used. Now it would require to peeling of the ClassTag that wraps the java.lang.Class.

Though I agree that this potential inefficiency can be eliminated by a very simple optimization. And thus I propose to not consider the performance of runtime code when discussing this PR.

@odersky
Copy link
Contributor Author

odersky commented Feb 19, 2016

Yes, it's too bad. These are good arguments, unfortunatly ;-) I close the PR in favor of #1105.

@odersky odersky closed this Feb 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants